Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bug fixes in matrix log #32327

Merged
merged 2 commits into from
Sep 23, 2019
Merged

bug fixes in matrix log #32327

merged 2 commits into from
Sep 23, 2019

Conversation

stevengj
Copy link
Member

Fixes #32313

@stevengj stevengj added bugfix This change fixes an existing bug linear algebra Linear algebra backport 1.0 labels Jun 14, 2019
@stevengj
Copy link
Member Author

Ugh, it looks like this PR triggers another problem. Will have to look at it more closely.

@RalphAS
Copy link

RalphAS commented Jun 15, 2019

As shown in the book excerpt you posted in #32313, these algorithms specifically exclude the case where eigenvalues are on the negative real line (which applies to some of the examples in related issues). Experiments with roundoff-negligible perturbations suggest that your proposed fix sometimes succeeds but is still fragile in that regime. Since eigenvalues are trivially available from the diagonal of the UpperTriangular, perhaps it would be appropriate to rotate the branch cut or issue a warning in such cases?

@stevengj
Copy link
Member Author

@RalphAS, where does the book say that?

@RalphAS
Copy link

RalphAS commented Jun 15, 2019

In Algorithm 11.10 (and in various papers on matrix log by Higham and associates) one finds the limitation "no eigenvalues on R-". On looking again I see that this is irrelevant to the issue you're addressing here and to the testing problems; sorry for the noise.

@RalphAS
Copy link

RalphAS commented Jun 15, 2019

I think there is a sign error in Eq. (11.27); to compensate for the atanh variance I believe you want
w = atanh(z) + im * pi * (unw(logAkp1-logAk) - unw(log1p(z)-log1p(-z)))

@KristofferC KristofferC mentioned this pull request Jul 1, 2019
Merged
32 tasks
@KristofferC KristofferC mentioned this pull request Jul 16, 2019
Merged
14 tasks
This was referenced Aug 26, 2019
Closed
Merged
@RalphAS RalphAS mentioned this pull request Sep 14, 2019
Merged
@RalphAS @andreasnoack
patches to matrix log (#33245)
db1cd35 
* patches to matrix log

Avoid integer overflow if `s > 63`.
Correct logic for `s == 0`.
Only use fancy divided difference formulae if eigenvalues are close - avoids dangerous roundoff error if they are in opposite sectors.

* add tests
@stevengj
Copy link
Member Author

Closed by #33245.

@stevengj stevengj closed this Sep 20, 2019
@martinholters
Copy link
Member

No? That was a PR into this branch, wasn't it?

@fredrikekre fredrikekre reopened this Sep 20, 2019
@andreasnoack
Copy link
Member

All test errors seem unrelated. cc @staticfloat

@andreasnoack andreasnoack merged commit 318affa into master Sep 23, 2019
@delete-merged-branch delete-merged-branch bot deleted the sgj/logfix branch September 23, 2019 07:05
@RalphAS RalphAS mentioned this pull request Sep 28, 2019
Merged
18 tasks
KristofferC pushed a commit that referenced this pull request Dec 3, 2019
@stevengj @KristofferC
bug fixes in matrix log (#32327)
b099946 
* bug fixes in matrix log

* patches to matrix log (#33245)

* patches to matrix log

Avoid integer overflow if `s > 63`.
Correct logic for `s == 0`.
Only use fancy divided difference formulae if eigenvalues are close - avoids dangerous roundoff error if they are in opposite sectors.

* add tests

(cherry picked from commit 318affa)
@KristofferC KristofferC mentioned this pull request Dec 3, 2019
Closed
56 tasks
KristofferC pushed a commit that referenced this pull request Feb 20, 2020
@stevengj @KristofferC
bug fixes in matrix log (#32327)
96b07a6 
* bug fixes in matrix log

* patches to matrix log (#33245)

* patches to matrix log

Avoid integer overflow if `s > 63`.
Correct logic for `s == 0`.
Only use fancy divided difference formulae if eigenvalues are close - avoids dangerous roundoff error if they are in opposite sectors.

* add tests

(cherry picked from commit 318affa)
KristofferC pushed a commit that referenced this pull request Feb 20, 2020
@stevengj @KristofferC
bug fixes in matrix log (#32327)
5f40a29 
* bug fixes in matrix log

* patches to matrix log (#33245)

* patches to matrix log

Avoid integer overflow if `s > 63`.
Correct logic for `s == 0`.
Only use fancy divided difference formulae if eigenvalues are close - avoids dangerous roundoff error if they are in opposite sectors.

* add tests

(cherry picked from commit 318affa)
KristofferC pushed a commit that referenced this pull request Feb 22, 2020
@stevengj @KristofferC
bug fixes in matrix log (#32327)
db0bc60 
* bug fixes in matrix log

* patches to matrix log (#33245)

* patches to matrix log

Avoid integer overflow if `s > 63`.
Correct logic for `s == 0`.
Only use fancy divided difference formulae if eigenvalues are close - avoids dangerous roundoff error if they are in opposite sectors.

* add tests

(cherry picked from commit 318affa)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bugfix This change fixes an existing bug linear algebra Linear algebra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

log(matrix) issue
5 participants
@stevengj @RalphAS @martinholters @andreasnoack @fredrikekre